I know there are other cases where we are doing this (In fact IIRC you implemented one at some point, and I think there may be others)? I might be making this up. If there are can we replace with this?

Out of curiosity what was wrong with the old version? Maybe a small comment on what all of this is for?

Unrelated, but it was causing test flakes, since the non-deterministic proto serialization led to distinct hashes for the same proto.

OK makes sense. Maybe just a small comment?

Is the reason we are passing std::string here vs reference string issues again? If so can we put a TODO somewhere?

I think this needs to get added to docs.

nit: worth defining Protobuf::MessagePtr? I think we will have a lot of this.

We still using any?

Just to check my understanding here: For a struct, whether JSON or proto, the type if embedded, so when it is actually parsed, it becomes the real object? Is that right?

The type doesn't appear in the config at all, all we have is a google.protobuf.Struct knowledge when we encounter an opaque config. We then convert this Struct to JSON and then load it back into a Protobuf::Message (superclass of all concrete proto types), by invoking Protobuf::util::JsonStringToMessage(json, &message). This function makes use of the type information in the supplied message to figure out how to do the parsing of the JSON.

This is why we need createConfigProto(), it's essentially providing a JSON->message conversion factory via the proto message it returns. If you can think of a cleaner way to do this, I'm open to that as well (we could literally renamed to createConfigProtoFactory and give it some more abstract interface if you prefer).

Does this mean that HttpConnectionManager message actually never appears on the wire, but is used internally by Envoy only? I.e., to pass in http connection manager config via a protobuf I have to map from the HttpConnectionManager message definition to a protobuf Struct, field by field. If so, what is the point of specifying the HttpConnectionMananger message in the envoy-api?

Could not figure it out, managed to get it to work by encoding to Struct, seems to work but looks quite inefficient coding-wise:

    filter_chains {
      filters {
        name: "http_connection_manager"
        config {
          fields {
            key: "rds"
            value {
              struct_value {
                fields {
                  key: "config_source"
                  value {
                    struct_value {
                      fields {
                        key: "api_config_source"
                        value {
                          struct_value {
                            fields {
                              key: "api_type"
                              value {
                                number_value: 2
                              }
                            }
                            fields {
                              key: "cluster_name"
                              value {
                                string_value: "rdsCluster"
                              }
                            }
                          }
                        }
                      }
                    }
                  }
                }
                fields {
                  key: "route_config_name"
                  value {
                    string_value: "router1"
                  }
                }
              }
            }
          }
          fields {
            key: "stat_prefix"
            value {
              string_value: "proxy"
            }
          }
        }
        deprecated_v1 {
          type: "read"
        }
      }
    }

Yes, it only appears as a Struct. What you have is correct. The goal here is not efficiency, but being extensible, we don't want to predefine all filter config protos upfront, we want to support user extension. The alternative google.protobuf.Any type was not attractive as it is hard to do anything with the encoded proto without the original schema, e.g. dumping debug, assembling in config generation tools.

This is why we use JSON/YAML for textual representation, the ugliness of Struct is hidden.

I'm interested in understanding the efficiency requirements here. Two questions:

1. Given this is control plane, I wouldn't normally be that concerned about slight encoding inefficiency. Do you have some very significant scaling requirements in terms of the size/frequency of config update?
2. I haven't looked at the wire format for Struct, but I assume the main objection is the fact that the key string is included, which is not needed if you know the proto upfront.

TBH, this is part of the API which we've tentatively frozen, so it would take a compelling reason to make this change.

I don't have any numbers now, but you are likely right in that performance difference does not matter here. Even so it seems to me that there is much bigger difference in encoding/decoding CPU/memory use than in the wire format. Given that one of the main rationales of protobufs vs. XML (for example) is >10x encoding/decoding performance difference this seems like going backwards.

On the second point the change could be made backwards compatible by keeping the code point 2 for Struct config, and adding new code point(s) for Any or Oneof Message configs?

I think the best thing to do at this point is open an issue and let's see what others in the community think.

@PiotrSikora

See #1680.

nit: Can we add variant of comment in counter() to here about thread safety.

Sorry I'm a little confused. I'm trying to understand why we need a createConfigProto() function in addition to the new style factory function. (I think I'm getting mixed up with how we are going from proto to JSON and then back to proto, etc.). I assume this is because we need a well typed structure to jam JSON into, but isn't that somehow known from the type inside it? Is there any way to make this more clear?

What you say above is correct, except for the bit about the "type inside". There's no implied type in a google.protobuf.Struct. If we had gone down the route of google.protobuf.Any, there would be a type URL present in the JSON that we could use to perform this conversion without the filter having to supply this factory (see the other comment on this topic for more discussion of JSON->message).

from json to yaml? and user configs have to mention these weird strings
"@type": type.googleapis.com/envoy.api.v2.Listener ? :(

Not sure what the "from json to yaml" is referring to, can you clarify?

On the @type annotations, these are a product of the generic handling of subscription types and ADS multiplexing support. I don't expect we'll see this stuff in hand-written configs in v2, instead, we will allow the bootstrap to specify both listeners and clusters, like v1, so this should only show up in test files like the above, or where folks want filesystem based dynamic config. I agree it's a bit ugly, but hand-written configs are really only supported in a best effort manner, one of the v2 API principles is that the API is primarily for machines.

Per offline convo I think we decided to switch to createEmptyConfigProto() and have a slightly more robust comment about struct -> JSON -> message. Otherwise change LGTM.